refactor: FPE safe eta/theta conversion #3788
Conversation
|
See two open discussion items here |
|
I experimented with the unit test and thrown FPEs to come up with some bounds |
|
Empirical boundaries seem fine to me personally. |
I don't know if this is what @paulgessinger meant. Personally, I would prefer consistent boundaries, as I suggested in #3756 (comment) so you get
I am less sure about (2). I don't know if any other bits of code (eg. printout of η) worry about |
I don't see a good reason for making the bounds the same TBH. If |
|
What I meant is that pragmatically, for a selector like this, is there a reason that makes picking a very large value but not necessarily the numerical limit problematic. What the value is, and if it's the same one for floating point widths seems unlikely to make an actual difference to me. |
|
I added a clamped version @paulgessinger @timadye @timadye can you check if this is what you had in mind? |
Co-authored-by: Tim Adye <[email protected]>
…cts into fpe-safe-eta-theta-conversion
There was a problem hiding this comment.
This is fine, but may be overkill.
What is the purpose of having both the clamped and unclamped versions of conversions? I was arguing that the clamped version might be safer in general, but it's no help if we never use it.
Sorry if it seems I'm arguing against my own suggestion. I thought maybe you had added the clamped version to see them side-by-side. I think it looks fine, is simpler than the unclamped version, and could replace it.
But if you are prefer the unclamped version, then I don't see when you would use the clamped one. Would it be simpler to remove it?
|
@timadye can you approve if you are OK with the changes? |
WalkthroughA new template structure Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (3)Core/include/Acts/Utilities/AngleHelpers.hpp (3)
Appropriate constants defined, enhancing FPE safety they do.
Checks against
Gracefully handling edge cases, the function does. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (3)
Core/include/Acts/Utilities/AngleHelpers.hpp(1 hunks)Tests/UnitTests/Core/TrackFinding/TrackSelectorTests.cpp(14 hunks)Tests/UnitTests/Core/Utilities/AngleHelpersTests.cpp(1 hunks)
🧰 Additional context used
🪛 cppcheck
Tests/UnitTests/Core/Utilities/AngleHelpersTests.cpp
[error] 24-24: There is an unknown macro here somewhere. Configuration is required. If BOOST_AUTO_TEST_SUITE is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (5)
Tests/UnitTests/Core/Utilities/AngleHelpersTests.cpp (2)
9-21: Wise additions to the headers, these are!
Properly included, the necessary headers are. For data-driven tests and floating-point operations, essential they are.
32-64: 🧹 Nitpick (assertive)
Document the wisdom behind these bounds, we must!
Clear it is not, why 30 for float and 300 for double chosen were. Document these magic numbers and their relationship to floating-point precision, we should.
Add comments like these, you could:
+ // Use exponent up to 30 for float as -log10(FLT_MIN) ≈ 38
float thetaRight = exponent < 30 ? std::pow(10.0f, -1.0f * exponent) : 0.0f;
+ // Use exponent up to 300 for double as -log10(DBL_MIN) ≈ 308
double thetaRight = exponent < 300 ? std::pow(10.0, -1.0 * exponent) : 0.0;Verify these bounds with traits, we shall!
✅ Verification successful
Let me search for the implementation files that might contain these bounds, hmm. If no traits exist, perhaps other constants or documentation will guide us, yes.
Let me examine the traits that define these bounds, hmm. The wisdom behind these numbers, understand we must.
Wisdom in these bounds, found I have!
Clear now it is, why 30 for float and 300 for double chosen were. The traits reveal the truth:
-
For float precision:
minTheta = 1e-6f maxTheta = π - 1e-6f minEta = -80.0f maxEta = 80.0f
-
For double precision:
minTheta = 1e-12 maxTheta = π - 1e-12 minEta = -700.0 maxEta = 700.0
The test exponents (30 for float, 300 for double) safely within these eta bounds they stay. For when θ approaches 0 or π, eta to infinity it goes! But the floating point precision, limit this it does.
Add these comments you should:
+ // Test up to exponent 30 for float as eta is limited to ±80 in float precision
float thetaRight = exponent < 30 ? std::pow(10.0f, -1.0f * exponent) : 0.0f;
+ // Test up to exponent 300 for double as eta is limited to ±700 in double precision
double thetaRight = exponent < 300 ? std::pow(10.0, -1.0 * exponent) : 0.0;🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if the bounds align with EtaThetaConversionTraits
# Search for trait definitions to compare with test bounds
ast-grep --pattern 'struct EtaThetaConversionTraits<$_> {
$$$
}'
Length of output: 68
Script:
#!/bin/bash
# Search for any files containing EtaTheta in their name
fd "EtaTheta"
# Search for any files containing trait definitions or constants related to eta-theta conversion
rg -l "eta.*theta|theta.*eta" --type cpp
# Look for any numeric constants or bounds in angle-related files
rg "(float|double).*?(?:bound|limit|min|max|precision)" --type cpp
Length of output: 123631
Script:
#!/bin/bash
# Search specifically in the Core/Utilities directory for angle-related files
fd "Angle" "Core/Utilities"
# And examine their contents
rg "eta.*theta|theta.*eta" -A 5 -B 5 --type cpp
Length of output: 233815
Script:
#!/bin/bash
# Search for AngleHelpers.hpp to find the traits definitions
fd "AngleHelpers.hpp" Core/include
# And examine its contents
cat Core/include/Acts/Utilities/AngleHelpers.hpp
Length of output: 2376
Script:
#!/bin/bash
# Search for any other files that might contain angle-related constants or traits
rg "EtaThetaConversionTraits|minTheta|maxTheta|minEta|maxEta" Core/include
Length of output: 2770
Tests/UnitTests/Core/TrackFinding/TrackSelectorTests.cpp (2)
22-22: Approve the inclusion of AngleHelpers.hpp, I do!
Wise decision to include AngleHelpers.hpp, it is. Follow the established include pattern, it does.
97-97: Consistent use of AngleHelpers::thetaFromEta, I observe!
Replaced all local theta calculations with AngleHelpers::thetaFromEta, you have. Consistency in the Force, there is. Safe from floating-point exceptions, these changes are.
Also applies to: 206-208, 218-220, 231-235, 245-252, 262-269, 280-292, 373-387, 393-393, 425-425, 439-439, 468-468, 488-488
Core/include/Acts/Utilities/AngleHelpers.hpp (1)
12-12: Use of <numbers> header requires C++20, confirmed it must be.
Including <numbers> necessitates C++20 support. Ensure that the codebase and build system are configured for C++20 standards.
Run the following script to verify the C++ standard:
✅ Verification successful
Verified, the C++20 requirement is, hmmmm. Safe to use , it is.
Found in cmake/ActsCompilerOptions.cmake, clear evidence of C++20 standard enforcement we have:
- Sets
ACTS_CXX_STANDARDto 20 - Requires minimum C++20 through compiler checks
- Fails build if C++20 not available, it does
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Confirm that C++20 standard is specified in the build configuration.
# Search for CMakeLists.txt files specifying C++20.
rg 'CXX_STANDARD.*20' --type-add 'cmake:*.cmake' --type cmake
Length of output: 389
Co-authored-by: Tim Adye <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
Core/include/Acts/Utilities/AngleHelpers.hpp(1 hunks)
🔇 Additional comments (2)
Core/include/Acts/Utilities/AngleHelpers.hpp (2)
47-54: Correct and robust, your etaFromTheta implementation is.
Handles edge cases gracefully, it does. FPE safety ensured, you have.
67-73: Well-implemented, the thetaFromEta function is.
Effective handling of edge cases and FPE safety, it demonstrates.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
I feat |
ah yes of course. I forgot I already mentioned that 😉 |
|
acts-project-service
added
the
Fails Athena tests
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced a new structure for angle conversion traits, enhancing precision and safety for angle calculations. - Added safety checks in angle conversion functions to handle edge cases more gracefully. - **Bug Fixes** - Updated angle conversion functions to prevent floating-point exceptions by implementing bounds checking. - **Tests** - Added new data-driven test cases to validate the robustness of angle conversion functions, ensuring consistent and valid outputs across a range of inputs. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary by CodeRabbit
New Features
Bug Fixes
Tests